-
Notifications
You must be signed in to change notification settings - Fork 411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(llmobs): submit spans for streamed calls #10908
base: main
Are you sure you want to change the base?
Conversation
|
Datadog ReportBranch report: ✅ 0 Failed, 1196 Passed, 0 Skipped, 30m 23.23s Total duration (6m 25.45s time saved) |
BenchmarksBenchmark execution time: 2024-10-02 20:31:35 Comparing candidate commit b2eef10 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 371 metrics, 53 unstable metrics. |
@@ -970,6 +968,9 @@ def _on_span_finished(span: Span, streamed_chunks): | |||
else: | |||
# best effort to join chunks together | |||
content = "".join([str(chunk) for chunk in streamed_chunks]) | |||
integration.llmobs_set_tags(span, args=args, kwargs=kwargs, response=content, operation="chain") | |||
if span.error or not integration.is_pc_sampled_span(span): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to set llm obs tags before we do the integration.is_pc_sampled_span
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question. it looks like we recently changed llmobs_set_tags
to run independently of is_pc_sampled_span
(example). followed the same logic here, but i think even before we did something like is_pc_sampled_llmobs
, which I think always returned True
.
@@ -970,6 +968,9 @@ def _on_span_finished(span: Span, streamed_chunks): | |||
else: | |||
# best effort to join chunks together | |||
content = "".join([str(chunk) for chunk in streamed_chunks]) | |||
integration.llmobs_set_tags(span, args=args, kwargs=kwargs, response=content, operation="chain") | |||
if span.error or not integration.is_pc_sampled_span(span): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another clarification question here about this - do we move the check for span.error
after setting tags because the span may contain an error that happened mid-stream but we still want to capture the outputs that were returned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we re-check span.error
in llmobs_set_tags
for llm
, chat
, and chain
, which i kept before adding output messages/value properties
What does this PR do?
Builds off of #10672, extending support for
llm.stream
,chat_model.stream
, andchain.stream
to LLM Observability. This PR tags those spans appropriately for LLM Observability, and marks them to submit to LLM Observability.Checklist
Reviewer Checklist